-
Notifications
You must be signed in to change notification settings - Fork 180
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for credential_process
from profiles
#1356
Add support for credential_process
from profiles
#1356
Conversation
Signed-off-by: Jesse Szwedko <jesse@szwedko.me>
credential_process
from profilescredential_process
from profiles
Signed-off-by: Jesse Szwedko <jesse@szwedko.me>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey all! I'll push this forward more next week but wanted to get it up for any early feedback.
@@ -493,4 +501,5 @@ mod test { | |||
make_test!(retry_on_error); | |||
make_test!(invalid_config); | |||
make_test!(region_override); | |||
make_test!(credentials_process); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will implement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we'll need to figure out how to mock this—probably some sort of "fake command" interface will need to get added to our existing OS shim
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, should validate that the credentials process can be used as the source profile for assume role
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I ended up using echo
for the test in which should be supported on all platforms, but happy to try to sub in command mocking if you prefer.
Ref: a787b61
Ah, also, I integrated this into the profile file provider rather than creating a new one that is part of the default chain. I think it makes sense as part of the profile file provider (this is how rusoto modeled it too) rather than a separate provider that also tries to load profile files, but open to opinions here. |
Signed-off-by: Jesse Szwedko <jesse@szwedko.me>
Adopt the AWS Java SDK approach of running through `cmd.exe` / `sh`. https://github.com/aws/aws-sdk-java/blob/fd409dee8ae23fb8953e0bb4dbde65536a7e0514/aws-java-sdk-core/src/main/java/com/amazonaws/auth/ProcessCredentialsProvider.java#L63-L82 Signed-off-by: Jesse Szwedko <jesse@szwedko.me>
Signed-off-by: Jesse Szwedko <jesse@szwedko.me>
Signed-off-by: Jesse Szwedko <jesse@szwedko.me>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall this is exactly the right direction! I may end up asking you to not include it in the default chain..by default. We need to figure out if there are any security safeguards needed to start using this generally, but I'll get back to you on that ASAP!
Great work on this so far, this looks awesome!
let mut command = Command::new(iter.next().ok_or_else(|| { | ||
CredentialsError::provider_error( | ||
"Error retrieving credentials from external process: provided command empty", | ||
) | ||
})?); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should spawn this into a task...the trouble is that we'd need to then abstract over spawning. I'll think about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I'm open to thoughts here. I actually up refactoring this to take the Java SDK's approach of just passing the whole string through sh -c
/ cmd.exe /C
in 14ad7a9 to avoid needing to do any parsing in the SDK itself.
let mut command = Command::new(iter.next().ok_or_else(|| { | ||
CredentialsError::provider_error( | ||
"Error retrieving credentials from external process: provided command empty", | ||
) | ||
})?); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should spawn this into a task...the trouble is that we'd need to then abstract over spawning. I'll think about it.
@@ -493,4 +501,5 @@ mod test { | |||
make_test!(retry_on_error); | |||
make_test!(invalid_config); | |||
make_test!(region_override); | |||
make_test!(credentials_process); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we'll need to figure out how to mock this—probably some sort of "fake command" interface will need to get added to our existing OS shim
@@ -100,6 +101,9 @@ impl ProviderChain { | |||
})? | |||
} | |||
BaseProvider::AccessKey(key) => Arc::new(key.clone()), | |||
BaseProvider::CredentialProcess(credential_process) => Arc::new( | |||
CredentialProcessProvider::new(credential_process.to_string()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if new
returned a result, we could return a profile file error here which is probably the right thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned this in #1356 (comment) but I actually ended up pulling out the command parsing logic, following suit with the Java SDK of just passing through the commands to sh -c
/ cmd.exe /C
.
@@ -100,6 +101,9 @@ impl ProviderChain { | |||
})? | |||
} | |||
BaseProvider::AccessKey(key) => Arc::new(key.clone()), | |||
BaseProvider::CredentialProcess(credential_process) => Arc::new( | |||
CredentialProcessProvider::new(credential_process.to_string()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if new
returned a result, we could return a profile file error here which is probably the right thing
@@ -493,4 +501,5 @@ mod test { | |||
make_test!(retry_on_error); | |||
make_test!(invalid_config); | |||
make_test!(region_override); | |||
make_test!(credentials_process); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, should validate that the credentials process can be used as the source profile for assume role
Signed-off-by: Jesse Szwedko <jesse@szwedko.me>
This required refactoring the existing JSON parsing a bit. Signed-off-by: Jesse Szwedko <jesse@szwedko.me>
Signed-off-by: Jesse Szwedko <jesse@szwedko.me>
Thanks for taking a look @rcoh ! I pushed some more changes and think this is ready for full review. |
@@ -177,7 +199,7 @@ pub(crate) fn parse_json_credentials( | |||
|
|||
pub(crate) fn json_parse_loop<'a>( | |||
input: &'a [u8], | |||
mut f: impl FnMut(Cow<'a, str>, Cow<'a, str>), | |||
mut f: impl FnMut(Cow<'a, str>, &Token<'a>) -> Result<(), InvalidJsonCredentials>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes to the JSON parsing to support IMDSv2, SSO, and the differing credential_process
JSON formats were a bit more awkward than I was hoping. I started with changing this loop function to return all value types since the credential_process
format has a number in it (Version
) which spidered out a bit.
@@ -115,7 +118,8 @@ pub(crate) fn parse_json_credentials( | |||
let mut expiration = None; | |||
let mut message = None; | |||
json_parse_loop(credentials_response.as_bytes(), |key, value| { | |||
match key { | |||
dbg!("calling loop with", &key, &value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget to change this to a tracing
call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dbg!
in this function probably aren't valuable to log and can just be removed.
CHANGELOG.next.toml
Outdated
@@ -22,3 +22,14 @@ message = "Log a debug event when a retry is going to be peformed" | |||
references = ["smithy-rs#1352"] | |||
meta = { "breaking" = false, "tada" = false, "bug" = false } | |||
author = "jdisanti" | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[[aws-sdk-rust]] |
missing the header for this entry
) | ||
.map_err(|_| { | ||
InvalidJsonCredentials::Other( | ||
"credential expiration time cannot be represented by a SystemTime".into(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused by this message. Is it saying "you gave us data but we couldn't build a SystemTime
from it" or "you gave us a SystemTime
but that's not valid for this"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably better to call it DateTime
in this instance since that's what it's getting parsed into.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Thanks a ton for putting in the work here.
I read through some internal documentation on this credential provider, which is the source of my redaction and process execution comments.
We should put // Security: <reason>
comments around things like manual Debug
implementations to avoid them getting converted back into derivations by a would be good intentioned, code simplifying, dev in the future 😃
/// [profile assume-role] | ||
/// credential_process = /opt/bin/awscreds-custom --username helen | ||
/// ``` | ||
CredentialProcess(&'a str), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The arguments to the credentials process are allowed to contain sensitive information, so we need to be careful not to log them by default. Thus, the Debug
implementation for this enum needs to do some amount of redaction on this value. I recommend creating a new-type around the &'a str
, and manually implementing Debug
for it such that only the part before the first space is output, followed by something like <args redacted>
if there was more to the string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for this—we should make something like:
struct<'a> Sensitive(Cow<'a, str>)
impl AsDeref<str> for Sensitive ...
we can then use it here and in the provider to avoid needing to manually do a full debug impl
#[derive(Debug)] | ||
pub struct CredentialProcessProvider { | ||
command: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment about Debug
argument redaction applies here.
)) | ||
})?; | ||
|
||
tracing::debug!(command = ?command, status = ?output.status, "executed command"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The command binary name can be logged at debug
, but the arguments must be logged at trace
since they can have sensitive information.
let mut command = if cfg!(windows) { | ||
let mut command = Command::new("cmd.exe"); | ||
command.args(&["/C", &self.command]); | ||
command | ||
} else { | ||
let mut command = Command::new("sh"); | ||
command.args(&["-c", &self.command]); | ||
command | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't need the if windows / else
since std::process::Command::new
will use the PATH
to find the executable if an absolute path wasn't given: https://doc.rust-lang.org/stable/std/process/struct.Command.html#method.new
The SDKs don't support things like shell aliases or argument variable substitution, so it shouldn't run through a shell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly, this is the way the Go V2 SDK and Java V2 SDKs are doing it. Boto3 looks like it does its own platform-based splitting, with its own implementation for Windows and using shlex
for other platforms. I'll dig into this some more to figure out what we should do for Rust.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not going to block on this since Go V2 and Java V2 are doing it this way.
if !output.status.success() { | ||
return Err(CredentialsError::provider_error(format!( | ||
"Error retrieving credentials from external process: exited with code: {}", | ||
output.status | ||
))); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case of a non-zero exit status, the contents of stderr
should be included in the returned error.
#[derive(Debug, PartialEq, Eq)] | ||
pub(crate) struct RefreshableCredentials<'a> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think JsonCredentials
implementing Debug
is actually a bug. We should either remove it, or add a manual Debug
implementation that redacts the credential values.
@@ -115,7 +118,8 @@ pub(crate) fn parse_json_credentials( | |||
let mut expiration = None; | |||
let mut message = None; | |||
json_parse_loop(credentials_response.as_bytes(), |key, value| { | |||
match key { | |||
dbg!("calling loop with", &key, &value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dbg!
in this function probably aren't valuable to log and can just be removed.
@@ -0,0 +1,6 @@ | |||
[default] | |||
source_profile = base | |||
credential_process = echo '{ "Version": 1, "AccessKeyId": "ASIARTESTID", "SecretAccessKey": "TESTSECRETKEY", "SessionToken": "TESTSESSIONTOKEN", "Expiration": "2022-05-02T18:36:00+00:00" }' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Good thinking.
"Expiration": "2022-05-02T18:36:00+00:00" | ||
*/ | ||
(key, Token::ValueNumber { value, .. }) if key.eq_ignore_ascii_case("Version") => { | ||
version = Some(value.to_u8()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to err on the safe side, the version should probably be an isize
to avoid weird error messages or behavior if someone puts a large number in. It wouldn't surprise me if someone decides version numbers will be dates in the future (e.g., 20220506
).
) | ||
.map_err(|_| { | ||
InvalidJsonCredentials::Other( | ||
"credential expiration time cannot be represented by a SystemTime".into(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably better to call it DateTime
in this instance since that's what it's getting parsed into.
any update on this functionality? |
I'll try and get this wrapped up and merged this week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
functionally, I think this is good to go. Before we merge, the main thing to get is the docs—the other comments can be a follow up, if preferred
match version { | ||
Some(1) => { | ||
let access_key_id = | ||
access_key_id.ok_or(InvalidJsonCredentials::MissingField("AccessKeyId"))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as a minor nit for readability, I'd probably handle the error cases & abort in the match then have the bulk of the code be un-nested
/// [profile assume-role] | ||
/// credential_process = /opt/bin/awscreds-custom --username helen | ||
/// ``` | ||
CredentialProcess(&'a str), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for this—we should make something like:
struct<'a> Sensitive(Cow<'a, str>)
impl AsDeref<str> for Sensitive ...
we can then use it here and in the provider to avoid needing to manually do a full debug impl
@@ -0,0 +1,6 @@ | |||
[default] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add a test where the command is something like exit 1
? And a test where it returns invalid JSON?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢
} | ||
} | ||
|
||
impl<T> Clone for CommandWithSensitiveArgs<T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is just #[derive(Clone)]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It needs the where T: Clone
clause, but maybe I can just put that bound on the struct itself and then derive...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I take that approach, then each other trait impl
needs to have a T: Clone
bound added, so I don't think it's much of a savings.
@jszwedko No worries, we're all busy people. Thanks again for submitting this PR and doing the bulk of the work on it! |
Motivation and Context
Fixes: awslabs/aws-sdk-rust#261
Description
Starting to add support for
credential_process
in the default credentials chain. I'm just pushing this up as a draft to get early feedback before continuing implementation.Testing
I tested this manually by using patching it into http://github.com/vectordotdev/vector but will follow through with tests here.
Checklist
CHANGELOG.next.toml
if I made changes to the smithy-rs codegen or runtime cratesCHANGELOG.next.toml
if I made changes to the AWS SDK, generated SDK code, or SDK runtime cratesBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.